Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

슬랙에서 멤버 DB 채우기 #20

Merged
merged 8 commits into from
Nov 9, 2023
Merged

슬랙에서 멤버 DB 채우기 #20

merged 8 commits into from
Nov 9, 2023

Conversation

shp7724
Copy link
Collaborator

@shp7724 shp7724 commented Oct 6, 2023

dev에 멤버 추가했어요

image

@shp7724 shp7724 requested a review from a team October 6, 2023 11:36
@shp7724 shp7724 self-assigned this Oct 6, 2023
@shp7724 shp7724 marked this pull request as ready for review October 16, 2023 14:25
Copy link
Collaborator

@minkyu97 minkyu97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨슴다

Comment on lines -33 to +34
return (
ROOT_PATH / f".env.{self.env}",
ROOT_PATH / f".env.{self.env}.local",
)
# Get from AWS Secret Manager
return ()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2) 아예 지우나요?? 로컬에서 테스트할 때는 dotenv로 하는 게 편하지 않을까요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.env.prod.env.dev는 secret manager 도입하기 전에 배포용으로 해뒀던건데, 이제 하나로 일원화하는게 좋을거같아서 지웠어요
로컬에서 테스트하고 싶을 때는 .env.local 입맛에 맞게 변경해서 쓰면 됩니당

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ㅇㅎ 그러면

ROOT_PATH / f".env.{self.env}.local",

이 부분은 남기는 게 맞지 않아요? 제가 dotenv 잘 몰라서 헛소리하는 것일 수도 있습니다 ㅎ..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zzz
.env.dev.local
.env.prod.local
두개를 없애고
.env.local
.env.test
이 두개만 사용하자~ 이말입니다.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아항~ ㅋㅋㅋ 친절한 설명 감사합니당

Comment on lines +20 to +21
def __init__(self, *args, **kwargs) -> None:
super().__init__(*args, **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2) *args, **kwargs 는 없애면 어떨까요?? 나중에 필요한 필드가 있다면 그때 추가해도 될 것 같아요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이거 근데,, 안해주면 에러가 떠요.
pydantic 내부적으로 따로 로직이 있는거같네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로그

Traceback (most recent call last):
  File "/Users/user/dev/waffledotcom-server/waffledotcom/src/batch/googleform/main.py", line 39, in <module>
    main()
  File "/Users/user/dev/waffledotcom-server/waffledotcom/src/batch/googleform/main.py", line 35, in main
    asyncio.run(solver.run(merge_google_form_active_members))
  File "/Users/user/.pyenv/versions/3.11.1/lib/python3.11/asyncio/runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/Users/user/.pyenv/versions/3.11.1/lib/python3.11/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/.pyenv/versions/3.11.1/lib/python3.11/asyncio/base_events.py", line 653, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/Users/user/dev/waffledotcom-server/waffledotcom/src/utils/dependency_solver.py", line 53, in run
    return await self.solve_command(request, dependant)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/dev/waffledotcom-server/waffledotcom/src/utils/dependency_solver.py", line 28, in solve_command
    values, errors, _1, _2, _3 = await solve_dependencies(
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/dev/waffledotcom-server/.venv/lib/python3.11/site-packages/fastapi/dependencies/utils.py", line 555, in solve_dependencies
    solved_result = await solve_dependencies(
                    ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/dev/waffledotcom-server/.venv/lib/python3.11/site-packages/fastapi/dependencies/utils.py", line 580, in solve_dependencies
    solved = await solve_generator(
             ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/dev/waffledotcom-server/.venv/lib/python3.11/site-packages/fastapi/dependencies/utils.py", line 505, in solve_generator
    return await stack.enter_async_context(cm)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/.pyenv/versions/3.11.1/lib/python3.11/contextlib.py", line 635, in enter_async_context
    result = await _enter(cm)
             ^^^^^^^^^^^^^^^^
  File "/Users/user/.pyenv/versions/3.11.1/lib/python3.11/contextlib.py", line 204, in __aenter__
    return await anext(self.gen)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/dev/waffledotcom-server/.venv/lib/python3.11/site-packages/fastapi/concurrency.py", line 36, in contextmanager_in_threadpool
    raise e
AttributeError: 'DBConfig' object has no attribute 'username'

Comment on lines +24 to +26
data = client.users_list().data

assert isinstance(data, dict)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3) 무조건 dict 인건가요? 맞으면 그냥 이렇게 한 줄로 어떤가요

Suggested change
data = client.users_list().data
assert isinstance(data, dict)
data: dict = client.users_list().data

P2) 만약 dict가 아닐 수도 있다면, assert 말고 적절한 에러를 던지고 이유를 로깅해주는 게 좋을 것 같아요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slack sdk가 type hinting이 제대로 안돼있어서 pylance가 빨간줄 띄우는거같아요

image

with open("./members_to_create.pickle", "wb") as f:
pickle.dump(members_to_create, f)

debug(members_to_create)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3) 처음 보는 코드인데... 뭐하는 함수인가요? 만약 단순 디버깅용이라면 debug 레벨로 출력되는 거 맞겠죠?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아,, 이건 그냥 debug print 예쁘게 해주는거예요
지워놓을게여

Comment on lines +52 to +54
def main():
solver = DependencySolver()
asyncio.run(solver.run(create_users_from_slack))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3) 이 부분 잘 이해가 안 가는데... batch 면 일정 주기마다 돌아가는 거 아닌가요?? 일회성으로 실행하는 것처럼 보이는데 부연 설명 좀 부탁합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스크립트는 일회성으로 실행하는게 맞지 않을까요
일정 주기로 돌리는건 cronjob이든 뭐든 외부에서 해주는게 맞을거같아요

Comment on lines +10 to +12
model_config = SettingsConfigDict(
case_sensitive=False, env_prefix="SLACK_", env_file=settings.env_files
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2) 만약 dotenv 아예 안 쓸 거면 여기 지워도 되지 싶습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아예 안쓰는건 아니고, .env.local, .env.test만 두려고 합니다

Copy link
Collaborator Author

@shp7724 shp7724 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minkyu97 매우 늦었네용.. 답글 달았슴다

Comment on lines -33 to +34
return (
ROOT_PATH / f".env.{self.env}",
ROOT_PATH / f".env.{self.env}.local",
)
# Get from AWS Secret Manager
return ()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.env.prod.env.dev는 secret manager 도입하기 전에 배포용으로 해뒀던건데, 이제 하나로 일원화하는게 좋을거같아서 지웠어요
로컬에서 테스트하고 싶을 때는 .env.local 입맛에 맞게 변경해서 쓰면 됩니당

Comment on lines +24 to +26
data = client.users_list().data

assert isinstance(data, dict)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slack sdk가 type hinting이 제대로 안돼있어서 pylance가 빨간줄 띄우는거같아요

image

with open("./members_to_create.pickle", "wb") as f:
pickle.dump(members_to_create, f)

debug(members_to_create)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아,, 이건 그냥 debug print 예쁘게 해주는거예요
지워놓을게여

Comment on lines +52 to +54
def main():
solver = DependencySolver()
asyncio.run(solver.run(create_users_from_slack))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스크립트는 일회성으로 실행하는게 맞지 않을까요
일정 주기로 돌리는건 cronjob이든 뭐든 외부에서 해주는게 맞을거같아요

Comment on lines +10 to +12
model_config = SettingsConfigDict(
case_sensitive=False, env_prefix="SLACK_", env_file=settings.env_files
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아예 안쓰는건 아니고, .env.local, .env.test만 두려고 합니다

@shp7724
Copy link
Collaborator Author

shp7724 commented Oct 31, 2023

@minkyu97 형한테 전달받은 활동회원 명단 기반으로 한번 더 데이터 채워넣엇슴다
근데 데이터 정제하기가 좀.... 애매한부분이 많아서 DB가 아주 깔끔한 상태는 아니긴 함

Copy link
Collaborator

@minkyu97 minkyu97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨음다~
확인은 안 해봤지만.. api 테스트도 된 거죠?

@shp7724
Copy link
Collaborator Author

shp7724 commented Nov 9, 2023

@minkyu97 api 테스트가 정확히 어떤걸 말하는지 모르겠지만, api 스펙을 제대로 만들진 않은거같아요
이건 프론트쪽이랑 좀 얘기를 해야할것같긴한데
일단 이건 머지하고 다른 pr로 할게욤

@shp7724 shp7724 merged commit c37ded9 into develop Nov 9, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants